Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: Introduce an upgrade command #2564

Merged
merged 12 commits into from
Apr 1, 2019
Merged

cli: Introduce an upgrade command #2564

merged 12 commits into from
Apr 1, 2019

Conversation

olix0r
Copy link
Member

@olix0r olix0r commented Mar 26, 2019

The install command errors when the deploy target contains an existing
Linkerd deployment. The upgrade command is introduced to reinstall or
reconfigure the Linkerd control plane.

Upgrade works as follows:

  1. The controller config is fetched from the Kubernetes API. The Public
    API is not used, because we need to be able to reinstall the control
    plane when the Public API is not available; and we are not concerned
    about RBAC restrictions preventing the installer from reading the
    config (as we are for inject).

  2. The install configuration is read, particularly the flags used during
    the last install/upgrade. If these flags were not set again during the
    upgrade, the previous values are used as if they were passed this time.
    The configuration is updated from the combination of these values,
    including the install configuration itself.

    Note that some flags, including the linkerd-version, are omitted
    since they are stored elsewhere in the configurations and don't make
    sense to track as overrides..

  3. The issuer secrets are read from the Kubernetes API so that they can
    be re-used. There is currently no way to reconfigure issuer
    certificates. We will need to create another workflow for
    updating these credentials.

  4. The install rendering is invoked with values and config fetched from
    the cluster, synthesized with the new configuration.

Fixes #2556

@olix0r olix0r self-assigned this Mar 26, 2019
@olix0r olix0r requested review from alpeb and ihcsim March 26, 2019 15:56
@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 26, 2019

Integration test results for 50e2109: success 🎉
Log output: https://gist.github.com/6fcbaef5aad0fbc234f7c9639f89efed

@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 26, 2019

Integration test results for e8db3ce: success 🎉
Log output: https://gist.github.com/d25fd58944e95bcc90b80613f7d5017b

cli/cmd/upgrade.go Outdated Show resolved Hide resolved
@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 26, 2019

Integration test results for 218d901: fail 😕
Log output: https://gist.github.com/74f87978b3427ce5a7b50c15c63758cd

cli/cmd/upgrade.go Outdated Show resolved Hide resolved
cli/cmd/upgrade.go Outdated Show resolved Hide resolved
cli/cmd/upgrade.go Outdated Show resolved Hide resolved
cli/cmd/upgrade.go Outdated Show resolved Hide resolved
cli/cmd/upgrade.go Outdated Show resolved Hide resolved
pkg/k8s/labels.go Show resolved Hide resolved
cli/cmd/upgrade.go Outdated Show resolved Hide resolved
olix0r added a commit that referenced this pull request Mar 27, 2019
This change moves resource-templating logic into a dedicated template,
creates new values types to model kubernetes resource constraints, and
changes the `--ha` flag's behavior to create these resource templates
instead of hardcoding the resource constraints in the various templates.

This is in service of #2564.
@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 28, 2019

Integration test results for cf88617: fail 😕
Log output: https://gist.github.com/15b55c74b24c04ae377149e33b035356

@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 28, 2019

Integration test results for 12741c8: fail 😕
Log output: https://gist.github.com/d590f8432d7197f467d366ff81a9f8f4

@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 28, 2019

Integration test results for 50f27e3: fail 😕
Log output: https://gist.github.com/74332eeff4f232e71b96a79ea9925c77

@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 28, 2019

Integration test results for 14b68ed: fail 😕
Log output: https://gist.github.com/5edbe1a126e4b00bd94a668d44447c7d

@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 29, 2019

Integration test results for ca41e5d: fail 😕
Log output: https://gist.github.com/d1cec473a1902f06226fe892912dcda7

@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 29, 2019

Integration test results for cb1e71e: success 🎉
Log output: https://gist.github.com/2bccd7baa86eaece378b6faae9273bfa

@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 29, 2019

Integration test results for 5ef3a0d: fail 😕
Log output: https://gist.github.com/5791346685dabfb79788e26f076ecf4a

The `install` command errors when the deploy target contains an existing
Linkerd deployment. The `upgrade` command is introduced to reinstall or
reconfigure the Linkerd control plane.

Upgrade works as follows:

1. The controller config is fetched from the Kubernetes API. The Public
   API is not used, because we need to be able to reinstall the control
   plane when the Public API is not available; and we are not concerned
   about RBAC restrictions preventing the installer from reading the
   config (as we are for inject).

2. The install configuration is read, particularly the flags used during
   the last install/upgrade. If these flags were not set again during the
   upgrade, the previous values are used as if they were passed this time.
   The configuration is updated from the combination of these values,
   including the install configuration itself.

   Note that some flags, including the linkerd-version, are omitted
   since they are stored elsewhere in the configurations and don't make
   sense to track as overrides..

3. The issuer secrets are read from the Kubernetes API so that they can
   be re-used. There is currently no way to reconfigure issuer
   certificates. We will need to create _another_ workflow for
   updating these credentials.

4. The install rendering is invoked with values and config fetched from
   the cluster, synthesized with the new configuration.
@olix0r olix0r marked this pull request as ready for review March 29, 2019 17:52
@olix0r olix0r changed the title WIP: cli: Introduce an upgrade command cli: Introduce an upgrade command Mar 29, 2019
@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 29, 2019

Integration test results for e813001: fail 😕
Log output: https://gist.github.com/25ed6354981cf960c4c618078b87700e

@grampelberg
Copy link
Contributor

Works for me, I tested:

  • stable -> this (via install)
  • this -> this (no options change)
  • this -> this + options (proxy + resource requests)

I turned --proxy-auto-inject on during the middle of this and can't figure out how to turn it off. Not a blocker, but something to write up an issue and fix later.

@olix0r
Copy link
Member Author

olix0r commented Mar 29, 2019

@grampelberg can you use upgrade --proxy-auto-inject=false ?

@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 29, 2019

Integration test results for 4095c7d: success 🎉
Log output: https://gist.github.com/4b6ddac7d3dfa62d2aa9c8eeab1a42f0

@grampelberg
Copy link
Contributor

That removes it from the output, I've not tried your --prune suggestion yet.

Copy link
Member

@siggy siggy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks (and works!) well. Had a few comments around structure, docs, and tests. 👍 🚢

@@ -0,0 +1,192 @@
package cmd
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider adding some tests for this file. we'll probably want integration tests as well, but that could be done in a subsequent PR.

Copy link
Member Author

@olix0r olix0r Apr 1, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in the interest of unblocking the release, i'm going to do this in a followup

pkg/k8s/labels.go Show resolved Hide resolved
cli/cmd/install.go Show resolved Hide resolved
cli/cmd/install.go Show resolved Hide resolved
cli/cmd/upgrade.go Show resolved Hide resolved
cli/cmd/upgrade.go Outdated Show resolved Hide resolved
@l5d-bot
Copy link
Collaborator

l5d-bot commented Mar 29, 2019

Integration test results for e23d85c: success 🎉
Log output: https://gist.github.com/c7181636e646ade24715efc5f77bd2f3

@alpeb
Copy link
Member

alpeb commented Mar 30, 2019

That removes it from the output, I've not tried your --prune suggestion yet.

So the suggestion was
linkerd upgrade --proxy-auto-inject=false | k apply -f - -l linkerd.io/control-plane-component --prune

We confirmed this works and removes the linkerd-proxy-injector deployment, but the mutating webhook config remains.

Apparently this doesn't affect the creation of new pods. But to be able to clean it up we need to add the linkerd.io/control-plane-component (tracked in #2602) and have that config be included in the install yaml (#2176)

Copy link
Member

@alpeb alpeb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good to me, modulo @siggy suggestions 👍

Copy link
Contributor

@ihcsim ihcsim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Just two comments.

Maybe in a future PR, we can consider if using sub-struct to organize the *Values and *Options will make the validateAndBuild() and validate() easier to follow (or not).

cli/cmd/upgrade.go Show resolved Hide resolved
// If we cannot determine whether the configuration exists, an error is returned.
func linkerdConfigAlreadyExistsInCluster() (bool, error) {
api, err := k8s.NewAPI(kubeconfigPath, kubeContext)
func exitIfClusterExists() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than the error handling, this looks very similar to the newK8s() and fetchConfigs() in upgrade.go. If we can remove the dependency on upgradeOption(), can we re-use them here?

 func exitIfClusterExists() {
-       kubeConfig, err := k8s.GetConfig(kubeconfigPath, kubeContext)
+       k, err := newK8s()
        if err != nil {
                fmt.Fprintln(os.Stderr, "Unable to build a Kubernetes client to check for configuration. If this expected, use the --ignore-cluster flag.")
                fmt.Fprintf(os.Stderr, "Error: %s\n", err)
                os.Exit(1)
        }

-       k, err := kubernetes.NewForConfig(kubeConfig)
+       _, err = fetchConfigs(k)
        if err != nil {
-               fmt.Fprintln(os.Stderr, "Unable to build a Kubernetes client to check for configuration. If this expected, use the --ignore-cluster flag.")
-               fmt.Fprintf(os.Stderr, "Error: %s\n", err)
-               os.Exit(1)
-       }
-
-       c := k.CoreV1().ConfigMaps(controlPlaneNamespace)
-       if _, err = c.Get(k8s.ConfigConfigMapName, metav1.GetOptions{}); err != nil {
                if kerrors.IsNotFound(err) {
                        return
                }

Copy link
Member

@siggy siggy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one nit then 🚢

@@ -224,7 +224,7 @@ func newCmdInstall() *cobra.Command {
cmd.PersistentFlags().AddFlagSet(flags)

// Some flags are not available during upgrade, etc.
cmd.PersistentFlags().AddFlagSet(options.installOnlyFlagSet(pflag.ExitOnError))
cmd.PersistentFlags().AddFlagSet(options.flagSet(pflag.ExitOnError))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think this is redundant with line 204: flags := options.flagSet(pflag.ExitOnError) ?

@l5d-bot
Copy link
Collaborator

l5d-bot commented Apr 1, 2019

Integration test results for 7068101: success 🎉
Log output: https://gist.github.com/e7025d4f186fbfda4aa0e3f49027da7f

@l5d-bot
Copy link
Collaborator

l5d-bot commented Apr 1, 2019

Integration test results for eead6d9: success 🎉
Log output: https://gist.github.com/ae7420a4fa92bfc3a395447a7275ddd7

@olix0r
Copy link
Member Author

olix0r commented Apr 1, 2019

CI passes but the issue isn't getting notified https://travis-ci.org/linkerd/linkerd2/jobs/514357190

@l5d-bot
Copy link
Collaborator

l5d-bot commented Apr 1, 2019

Integration test results for 374562e: success 🎉
Log output: https://gist.github.com/106a023318627ce0ea94c4f22beea5ef

@olix0r olix0r merged commit d74ca1b into master Apr 1, 2019
@olix0r olix0r deleted the ver/upgrade branch April 1, 2019 20:27
@admc admc mentioned this pull request Apr 2, 2019
25 tasks
@siggy siggy mentioned this pull request Apr 4, 2019
@klingerf klingerf mentioned this pull request Apr 4, 2019
KatherineMelnyk pushed a commit to KatherineMelnyk/linkerd2 that referenced this pull request Apr 5, 2019
The `install` command errors when the deploy target contains an existing
Linkerd deployment. The `upgrade` command is introduced to reinstall or
reconfigure the Linkerd control plane.

Upgrade works as follows:

1. The controller config is fetched from the Kubernetes API. The Public
   API is not used, because we need to be able to reinstall the control
   plane when the Public API is not available; and we are not concerned
   about RBAC restrictions preventing the installer from reading the
   config (as we are for inject).

2. The install configuration is read, particularly the flags used during
   the last install/upgrade. If these flags were not set again during the
   upgrade, the previous values are used as if they were passed this time.
   The configuration is updated from the combination of these values,
   including the install configuration itself.

   Note that some flags, including the linkerd-version, are omitted
   since they are stored elsewhere in the configurations and don't make
   sense to track as overrides..

3. The issuer secrets are read from the Kubernetes API so that they can
   be re-used. There is currently no way to reconfigure issuer
   certificates. We will need to create _another_ workflow for
   updating these credentials.

4. The install rendering is invoked with values and config fetched from
   the cluster, synthesized with the new configuration.

Signed-off-by: [email protected] <[email protected]>
KatherineMelnyk pushed a commit to KatherineMelnyk/linkerd2 that referenced this pull request Apr 5, 2019
The `install` command errors when the deploy target contains an existing
Linkerd deployment. The `upgrade` command is introduced to reinstall or
reconfigure the Linkerd control plane.

Upgrade works as follows:

1. The controller config is fetched from the Kubernetes API. The Public
   API is not used, because we need to be able to reinstall the control
   plane when the Public API is not available; and we are not concerned
   about RBAC restrictions preventing the installer from reading the
   config (as we are for inject).

2. The install configuration is read, particularly the flags used during
   the last install/upgrade. If these flags were not set again during the
   upgrade, the previous values are used as if they were passed this time.
   The configuration is updated from the combination of these values,
   including the install configuration itself.

   Note that some flags, including the linkerd-version, are omitted
   since they are stored elsewhere in the configurations and don't make
   sense to track as overrides..

3. The issuer secrets are read from the Kubernetes API so that they can
   be re-used. There is currently no way to reconfigure issuer
   certificates. We will need to create _another_ workflow for
   updating these credentials.

4. The install rendering is invoked with values and config fetched from
   the cluster, synthesized with the new configuration.

Signed-off-by: [email protected] <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants